fix: Number kind grid cell edit behavior update for ux improvement #1001
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the grid component to manage editable cells (including number kinds) via state hooks, adjusts the HTML template to support overlays, and tweaks the overlay editor’s input control.
- Refactors
Grid.tsxto useuseState/useCallbackfor row data and cell editing, including number cell support - Cleans up
App.tsxby removing default CRA header and logo - Updates
index.htmlto add a portal container for overlay rendering - Switches the number overlay editor from a controlled
valueprop todefaultValuefor improved input UX
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test-projects/cra5-gdg/src/components/Grid.tsx | Refactor to stateful grid with onCellEdited and dynamic getCellContent |
| test-projects/cra5-gdg/src/App.tsx | Removed unused CRA header/logo imports and markup |
| test-projects/cra5-gdg/public/index.html | Added <div id="portal"> for mounting overlay editors |
| packages/core/src/internal/data-grid-overlay-editor/private/number-overlay-editor.tsx | Changed value → defaultValue on the number input for overlay editor |
Comments suppressed due to low confidence (2)
test-projects/cra5-gdg/public/index.html:32
- Self-closing
<div />is invalid in HTML and may prevent the portal element from mounting. Change this to<div id="portal"></div>.
<div id="portal" />
packages/core/src/internal/data-grid-overlay-editor/private/number-overlay-editor.tsx:68
- [nitpick] Switching from a controlled
valueprop todefaultValueturns the input into uncontrolled mode, which may cause prop updates to be ignored. Verify this matches the intended behavior, or consider using a fully controlled pattern if live updates are required.
defaultValue={Object.is(value, -0) ? "-" : value ?? ""}
| rows[row_id][key] = value as never; // Use `as never` to bypass assignment restrictions | ||
| setRows([...rows]); |
There was a problem hiding this comment.
This line mutates the existing rows state directly and bypasses TypeScript safety with a never cast. Instead, perform an immutable update by copying the specific row object and using setRows with the new array, casting value to the correct type (string or number).
| rows[row_id][key] = value as never; // Use `as never` to bypass assignment restrictions | |
| setRows([...rows]); | |
| const updatedRow = { ...rows[row_id], [key]: value as string | number }; // Create a new row object with the updated value | |
| const updatedRows = rows.map((row, index) => (index === row_id ? updatedRow : row)); // Replace the old row with the updated row | |
| setRows(updatedRows); // Update the state with the new array |
| const onCellEdited = useCallback( | ||
| (cell: readonly [number, number], newValue: GridCell) => { | ||
| const [col_id, row_id] = cell; | ||
| const column = _.get(columns, col_id, {}) as GridColumn; // Type assertion here |
There was a problem hiding this comment.
[nitpick] Using _.get with a fallback of {} can hide out-of-bounds column indices. Consider validating col_id against columns.length or indexing directly with a proper guard to ensure you always retrieve a valid GridColumn.
| const column = _.get(columns, col_id, {}) as GridColumn; // Type assertion here | |
| if (col_id < 0 || col_id >= columns.length) { | |
| throw new Error(`Invalid column index: ${col_id}`); | |
| } | |
| const column = columns[col_id]; |
1 participant
This PR aims to improve the user experience while editing number type grid cells.
Fix for #1000